-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support regex usage in Azure Service Bus scaler. #3607
Conversation
That's true but as soon as we support trigger ordering for evaluation or related cross-trigger features this goes away. I do agree regex has its place, but maybe we need both? (list + regex?) What are your thoughts @zroubalik @JorTurFer? I personally wouldn't like to use regex as a list is simpler but see value in both. |
For me, regex is totally enough, it's the approach we have in RabbitMQ, but I'm not against supporting both |
This is another one of those where I'm being a pain to try to increase the UX for non deep-experts |
I feel having multiple parameters to do the same thing makes it more confusing for users. The regex approach also maintains parity with RabbitMQ. |
Any more comments here @JorTurFer @tomkerkhove? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Should we add any e2e test for this?
I'll add it. Should we cover for both queue and subscriber cases? (I don't mind adding them, I am just thinking about how the test suite size). |
If possible, I'd test both, but IDK how complicated it is. Maybe if we add more test to azure, it's time also to create a folder to group all azure e2e test like we do with rabbit or redis |
be62ef9
to
f77684d
Compare
/run-e2e azure |
PTAL @JorTurFer |
What is the operation for? Not sure I get the usecase for it |
Given that multiple queues / subscriptions can match the regex, we need an aggregation operation to convert the message counts into a single metric. |
let's wait till the SDK update is merged (sorry for the extra work :( ) |
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
Signed-off-by: Vighnesh Shenoy <[email protected]>
f77684d
to
550e364
Compare
Updated after the SDK change. PTAL @JorTurFer |
/run-e2e azure |
/run-e2e azure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I chose regex instead of supporting a list in
queueName
/subscriptionName
parameters. Reason for this being, if the number of entities is small enough to write in a list, the user might as well use multiple triggers within theScaledObject
. While regex will allow users to specify a large number of entities easily.Checklist
Fixes #1624
Fixes #2920
Relates to kedacore/keda-docs#917